Use Enums for SetupOutputVariable Arguments (Try 2) #8898

Myoldmopar · 2021-07-19T17:33:04Z

Pull request overview

Original effort on Enumerate SetupOutputVariable Arguments #8879, however, the scope of the PR slipped and ended up causing change in outputs and was removed
For this PR, we are simply converting the string arguments into SetupOutputVariable from strings to enums. We are not trying to reduce the number of them, or coerce them into the existing StoreType and TimeStepType enums. That can be on a more targeted PR later.

As is, this should not cause any diffs or user-facing impact at all.

… up unit tests accordingly

Myoldmopar

Code walkthrough completed. It is a pretty straightforward set of changes from string arguments to an enumerated list that "matches" the original string list. This should be a clean CI result and be able to slide into develop easily. I got zero diffs on the full regression suite locally and unit test suite passes.

Myoldmopar · 2021-07-19T17:33:41Z

src/EnergyPlus/BaseboardElectric.cc

-                                "System",
-                                "Sum",
+                                OutputProcessor::SOVTimeStepType::System,
+                                OutputProcessor::SOVStoreType::Summed,


This is the kind of change in this PR -- lots of SetupOutputVariable calls changed from strings to the new enums.

Myoldmopar · 2021-07-19T17:34:09Z

src/EnergyPlus/BoilerSteam.cc

+                            this->BoilerMassFlowRate,
+                            OutputProcessor::SOVTimeStepType::System,
+                            OutputProcessor::SOVStoreType::Average,
+                            this->Name);


Some of the changes caused Clang Format to require line breaks.

Myoldmopar · 2021-07-19T17:35:54Z

src/EnergyPlus/OutputProcessor.hh

+        Average,
+        Num
+    };
+    constexpr std::array<std::string_view, (int)SOVStoreType::Num> sovStoreTypeStrings = {"State", "NonState", "Summed", "Average"};


New enumerations that are based directly on the original strings passed in. In a future effort, we need to evaluate whether these should be kept or not, and try to move to the StoreType and TimeStepType enums below in this file. However, if we remove those, then we lose the ability to differentiate between System, HVAC, Zone, and Plant on the TimeStepType, and those are still seen user-side in the SQL output indexGroup.

Well, OK for now, but indexGroup is a completely different animal from TimeStepType and StoreType. TimeStepType should only be zone or HVAC (or system of whatever we choose). indexGroup is a larger set with a few common names. Perhaps these were sharing the same list before, so when it was reduced it impacted both parameters.

Myoldmopar · 2021-07-19T17:36:26Z

src/EnergyPlus/OutputProcessor.hh

@@ -561,8 +583,8 @@ namespace OutputProcessor {
    void InitializeOutput(EnergyPlusData &state);

    void SetupTimePointers(EnergyPlusData &state,
-                           std::string const &IndexKey, // Which timestep is being set up, 'Zone'=1, 'HVAC'=2
-                           Real64 &TimeStep             // The timestep variable.  Used to get the address
+                           OutputProcessor::SOVTimeStepType const &IndexKey, // Which timestep is being set up, 'Zone'=1, 'HVAC'=2


Modified this worker function to use the new enums instead of strings.

Myoldmopar · 2021-07-19T17:36:45Z

src/EnergyPlus/OutputProcessor.hh

+                         std::string const &VariableName,                         // String Name of variable (with units)
+                         OutputProcessor::Unit const &VariableUnit,               // Actual units corresponding to the actual variable
+                         Real64 &ActualVariable,                                  // Actual Variable, used to set up pointer
+                         OutputProcessor::SOVTimeStepType const &TimeStepTypeKey, // Zone, HeatBalance=1, HVAC, System, Plant=2


Modified the SetupOutputVariable function declarations for the new enums.

Myoldmopar · 2021-07-19T17:39:18Z

src/EnergyPlus/OutputProcessor.cc

-        // TODO: , "HEATBALANCE", "HEAT BALANCE" are used nowhere aside from tests. Should we remove them?
-        std::vector<std::string> zoneIndexes({"ZONE", "HEATBALANCE", "HEAT BALANCE"});
-        std::vector<std::string> systemIndexes({"HVAC", "SYSTEM", "PLANT"});
-        std::string uppercase(UtilityRoutines::MakeUPPERCase(TimeStepTypeKey));


Got rid of these vectors and loops and just replaced it with a switch block now that the input parameter is an enum and not a string.

Myoldmopar · 2021-07-19T17:39:45Z

src/EnergyPlus/OutputProcessor.cc

@@ -969,7 +958,7 @@ namespace OutputProcessor {
        return StandardTimeStepTypeKey;
    }

-    StoreType validateVariableType(EnergyPlusData &state, std::string const &VariableTypeKey)
+    StoreType validateVariableType(EnergyPlusData &state, OutputProcessor::SOVStoreType const &VariableTypeKey)


Same comments as for validateTimStepType, but this time for StoreType.

Myoldmopar · 2021-07-19T17:40:57Z

src/EnergyPlus/OutputProcessor.cc

@@ -5630,7 +5607,7 @@ void SetupOutputVariable(EnergyPlusData &state,
                                                  thisVarPtr.storeType,
                                                  thisVarPtr.ReportID,
                                                  localIndexGroupKey,
-                                                  TimeStepTypeKey,
+                                                  std::string(sovTimeStepTypeStrings[(int)TimeStepTypeKey]),


Using a string representation of the time step type enum when passing in the indexGroup. This is where the HVAC, Zone, System, Plant difference is noted on the output. If you collapse TimeStepTypeKey into just HVAC and Zone, then this parameter will only be one of those two values, and then the output will be changed...

Ah yes, this is where the problem lies. I don't think this should be choosing from TimeStepTypeKey, they are different sets of options.

Absolutely, this is where the problems began on that last PR. If we fix this up independently then going back in and pushing the enum work forward will be super easy.

Myoldmopar · 2021-07-19T17:41:18Z

src/EnergyPlus/SimulationManager.cc

-        SetupTimePointers(state, "HVAC", TimeStepSys);
+        SetupTimePointers(
+            state, OutputProcessor::SOVTimeStepType::Zone, state.dataGlobal->TimeStepZone); // Set up Time pointer for HB/Zone Simulation
+        SetupTimePointers(state, OutputProcessor::SOVTimeStepType::HVAC, TimeStepSys);


Modified the calls to SetupTimePointers to use the new enums.

Myoldmopar · 2021-07-19T17:41:56Z

tst/EnergyPlus/unit/CondenserLoopTowers.unit.cc

-    OutputProcessor::SetupTimePointers(*state, "HVAC", state->dataHVACGlobal->TimeStepSys);
+    OutputProcessor::SetupTimePointers(
+        *state, OutputProcessor::SOVTimeStepType::Zone, state->dataGlobal->TimeStepZone); // Set up Time pointer for HB/Zone Simulation
+    OutputProcessor::SetupTimePointers(*state, OutputProcessor::SOVTimeStepType::HVAC, state->dataHVACGlobal->TimeStepSys);


Unit test files modified accordingly....I wonder if these calls here are actually necessary. Not in scope to investigate that here.

dareumnam

Edwin, Thanks for this PR. I have a quick question: does this PR have like getEnumerationIndex() function?

amirroth · 2021-07-20T16:33:07Z

src/EnergyPlus/OutputProcessor.hh

-                         Optional_int_const indexGroupKey = _,      // Group identifier for SQL output
-                         Optional_string_const customUnitName = _   // the custom name for the units from EMS definition of units
+                         std::string const &VariableName,                         // String Name of variable (with units)
+                         OutputProcessor::Unit const &VariableUnit,               // Actual units corresponding to the actual variable


Don't pass enums by reference unless you are actually returning a value through this parameter. Generally, the only parameters you should pass by reference are those you are using to return a value or objects that you don't want to copy from the caller to the callee (pass these by const reference). Pass everything else by value. Passing by reference unnecessarily is slower and also impedes compiler optimization.

@amirroth Thanks for looking it over and I hear you. I have to clean up a compiler warning so I'll make this change then.

In general though, on this second attempt of this effort I was just trying to be as minimalistic as I could to avoid causing any diffs and holding this up further, fully understanding that there will be a pass that collects the enums into a smaller list and improves it in other ways as well.

Myoldmopar · 2021-07-20T17:00:04Z

Edwin, Thanks for this PR. I have a quick question: does this PR have like getEnumerationIndex() function?

@dareumnam No, for this redo of the branch I stuck with a very minimalistic implementation. Just replacing the string arguments with matching enum arguments. I did add a constexpr array of strings that match the enum indexes to get strings, but I never ended up needing an "enum from string" function for this.

amirroth · 2021-07-20T17:15:02Z

@Myoldmopar, would be good to add gsl::span and getEnumerationValue function either as part of this PR or a different one so that we can start rolling that pattern out elsewhere even if we are not using it here.

Myoldmopar · 2021-07-20T17:27:19Z

I can do that:

Clean up compiler warning
Change enum args to pass by value
Add gsl::span into third_party
Implement int getEnumerationValue(std::span<std::string_view> sList, std::string_view s) in, perhaps, UtilityRoutines.hh, such that this type of stuff works: https://godbolt.org/z/bnbe9Gcsr -- except call SameString for case insensitivity, and may need a string_view implementation of SameString.

Let me know if you want anything else added on here.

amirroth · 2021-07-20T18:07:19Z

That is great, thanks.

Myoldmopar · 2021-07-21T14:20:36Z

I think I've got it all addressed. I'm building locally and applying formatting stuff so this should be all clean once CI makes another pass. I'll be out for a bit but by iteration call time we should know if this is ready to drop in.

Myoldmopar · 2021-07-21T19:08:46Z

Silly compiler warnings...they are cleaned up locally and I'll push shortly. Then this should be ready to go in. @dareumnam @amirroth here's your getEnumerationValue function, let me know if you spot any remaining issues here.

Myoldmopar · 2021-07-21T19:16:33Z

Also fixed the LaTeX warnings -- I had lumped those fixes into the other enum branch that got deleted so just re-did those changes in this branch.

jasondegraw · 2021-07-21T19:28:46Z

src/EnergyPlus/UtilityRoutines.cc

+        if (UtilityRoutines::SameString(sList[i], s)) return i;
+    return -1;
+}
+


Is this the function that's supposed to get used to match input enum values against the accepted values?

Yes. Create a constexpr std::array<std::string_view> that has the strings corresponding to the enumeration and pass it to this function. The use of gsl::span (the artist known as std::span in C++20) allows you to pass std::array of different sizes to the same function without templating. The same array can also be used to convert the enumeration back to string using direct lookup.

OK. In the input case, shouldn't this assert or something similar rather than return -1? Since the input values are validated by the input processor, so if the value isn't found there must be a mismatch between what's in the input schema and what's in the array.

So, it should return -1 (@jasondegraw, this is not the hill you want to die on). Whether it should also do something else is debatable.

EnergyPlus generally doesn't assert silently, it calls ShowSevereError or ShowFatalError. For input processing, that error message usually has some context information like the object type and name and field name. To preserve this kind of diagnostic, we would need to pass this context information to the function. Which we could do. Or we could create a wrapper function that takes this additional context and also prints a message. Or we could keep that logic at the call sites.

The tradeoff above is setting up and passing additional arguments for the rare case in which there is an error on one hand, and decluttering call-sites on the other.

There's a larger discussion to be had about error handling in EnergyPlus. Right now, the paradigm is to "accumulate" and track errors using ErrorFound parameters that are threaded through many functions, and then at various points to check that parameter and call ShowFatalError and give up the ghost. At some point we may want to switch to using exceptions. "Accumulating" errors is a bit trickier in that case, but it could be made to work. We would have to be careful to not throw exceptions across the API.

I stopped adding the else to my code, but I think there is a lot still there that's left over from pre-epJSON days. Some is hiding in the pre-epJSON style input.

Okay. At some point in the near future, we will start to see the second and subsequent phases of the enum transition roll in, including use of constexpr std::array<std::string_view>, getEnumerationValue, and switch/case instead of string-based if-else-if ladders. We can remove these else clauses at that time. No point in doing the same checks and printing similar errors twice.

I appreciate all this conversation. For the sake of this PR, I was asked to add in GSL::span so that we could start building on it. I put in this function as a demonstration only, I'm not even using this new function. If I'm picking up the mood of this thread, it feels like this PR doesn't necessarily need to change, but when we start using this function in another branch, we may want to consider some slight modification.

If I am correct there, can we get this conversation moved to a Discussion or something and point back to this comment block? However, if I need to change it here to get this PR in, let me know and I'm happy to alter it.

No need to change the PR. The last several messages have been about how to use the PR in the (near) future. Sorry.

Myoldmopar · 2021-07-22T15:51:52Z

CI is a bit sloppy but it appears to be unrelated. FWIW, the Linux performance results show a 0.5% performance gain from using these enums. I know that doesn't exactly reflect wall time, but it's still a positive. The Mac unit test failure pops up occasionally (I'm about to skip that unit test so that it doesn't accidentally mask a real failure), and the GPU diffs are unrelated obviously.

amirroth · 2021-07-22T15:58:08Z

CI is a bit sloppy but it appears to be unrelated. FWIW, the Linux performance results show a 0.5% performance gain from using these enums. I know that doesn't exactly reflect wall time, but it's still a positive.

Integers FTW!

Myoldmopar · 2021-07-22T16:01:57Z

This appears ready. Last call for commentary before it merges this afternoon.

Myoldmopar · 2021-07-22T20:02:07Z

Merging and moving on...

Myoldmopar added 3 commits July 19, 2021 11:44

Modified SetupOutputVariable functions to use enums

c39294e

Convert all SetupOutputVariable calls (and associated functions), fix…

0d8c733

… up unit tests accordingly

Clang format

dc4eca0

Myoldmopar added the Refactoring Includes code changes that don't change the functionality of the program, just perform refactoring label Jul 19, 2021

Myoldmopar requested a review from dareumnam July 19, 2021 17:33

Myoldmopar commented Jul 19, 2021

View reviewed changes

dareumnam reviewed Jul 20, 2021

View reviewed changes

mitchute referenced this pull request Jul 20, 2021

OutConvClass to enum class, fix unit tests

c59ca83

amirroth reviewed Jul 20, 2021

View reviewed changes

Myoldmopar added 5 commits July 20, 2021 14:12

Cleanup ValidateTimeStepType, minimal other clang-tidy things

f64c0be

Remove unused SetupOutputVariable overload

52bdced

Cleaning up minor header IDE warnings

ca5c63f

Continued cleaning up minor warnings

ddd25aa

Add GSL span and getEnumerationValue function

cdd4de1

Myoldmopar added 2 commits July 21, 2021 09:22

Modify arg types and fix unit test build issue

fe68a70

Clang-format enum branch

3b36239

Fix compiler warnings and fix latex warnings

660f580

Clang Format enum branch again

4cf80fe

jasondegraw reviewed Jul 21, 2021

View reviewed changes

mjwitte mentioned this pull request Jul 22, 2021

Allow zone hvac to draw return air #8719

Merged

17 tasks

Myoldmopar merged commit a6fa2f3 into develop Jul 22, 2021

Myoldmopar deleted the SetupOutputVariableEnums2 branch July 22, 2021 20:02

Use Enums for SetupOutputVariable Arguments (Try 2) #8898

Use Enums for SetupOutputVariable Arguments (Try 2) #8898

Conversation

Myoldmopar commented Jul 19, 2021

Pull request overview

Myoldmopar left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dareumnam left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Myoldmopar commented Jul 20, 2021

amirroth commented Jul 20, 2021

Myoldmopar commented Jul 20, 2021 • edited Loading

amirroth commented Jul 20, 2021

Myoldmopar commented Jul 21, 2021

Myoldmopar commented Jul 21, 2021

Myoldmopar commented Jul 21, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

amirroth Jul 22, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Myoldmopar commented Jul 22, 2021

amirroth commented Jul 22, 2021

Myoldmopar commented Jul 22, 2021

Myoldmopar commented Jul 22, 2021

Myoldmopar commented Jul 20, 2021 •

edited

Loading

amirroth Jul 22, 2021 •

edited

Loading